Migrate Composite component from reakit to ariakit
#54225
Conversation
Making sure we have sufficient coverage to be confident that any changes don't break this component.
Temporarily writing it to v2 so other components continue to work while this is ongoing.
|
Hi @jsnajdr, @diegohaz – hoping I can get some ideas. I'm in the process of migrating a Gutenberg component from The two test runs below (skipped irrelevant bits) are consecutive, with no changes, and fail in different places. This seems to be because sometimes initial focus is in different places, and not where the component expects it to be. As it's not consistent, I'm figuring it's something in my tests, or me missing something in how Essentially, there's a three-by-three grid, and by default the focus should be in the center. This always seems to be fine in practice, but in the unit tests sometimes that's true and other times it's in the top left. I wrote the tests against |
|
@andrewhayward If everything works in the browser, but fails in the tests, I'd try to start replacing the following If that doesn't work, it may be necessary to This happens because those unit tests are mostly synchronous, and the actual behavior may happen at a different timing (for example, using More context: #52133 (comment) |
Thanks for the suggestions, @diegohaz. The only way I was able to get the tests to all pass consistently was with three const asyncRender = async ( jsx ) => {
const view = render( jsx );
await act( async () => {
await new Promise( requestAnimationFrame );
await new Promise( requestAnimationFrame );
await new Promise( requestAnimationFrame );
} );
return view;
};Never a big fan of "magic" like this, but it seems like it's what the underlying implementation needs. |
There was a problem hiding this comment.
I think that we can levegra ariakit's internal store logic more than we're currently doing — had a quick attempt at that on my machine and came up with these changes (they may not be prefect, but they may be a step in the right direction?)
Click to expand
diff --git a/packages/components/src/alignment-matrix-control/index.tsx b/packages/components/src/alignment-matrix-control/index.tsx
index 1d8a1a08c4..20f2c4770c 100644
--- a/packages/components/src/alignment-matrix-control/index.tsx
+++ b/packages/components/src/alignment-matrix-control/index.tsx
@@ -8,7 +8,6 @@ import classnames from 'classnames';
*/
import { __, isRTL } from '@wordpress/i18n';
import { useInstanceId } from '@wordpress/compose';
-import { useState } from '@wordpress/element';
/**
* Internal dependencies
@@ -17,7 +16,7 @@ import Cell from './cell';
import { Composite, CompositeRow, useCompositeStore } from '../composite/v2';
import { Root, Row } from './styles/alignment-matrix-control-styles';
import AlignmentMatrixControlIcon from './icon';
-import { GRID, getItemId, normalizeValue } from './utils';
+import { GRID, getItemId, getItemValue } from './utils';
import type { WordPressComponentProps } from '../ui/context';
import type {
AlignmentMatrixControlProps,
@@ -56,27 +55,30 @@ export function AlignmentMatrixControl( {
width = 92,
...props
}: WordPressComponentProps< AlignmentMatrixControlProps, 'div', false > ) {
- const [ immutableDefaultValue ] = useState(
- normalizeValue( value ?? defaultValue )
- );
- const currentCell = normalizeValue( value ?? immutableDefaultValue );
-
const baseId = useInstanceId(
AlignmentMatrixControl,
'alignment-matrix-control',
id
);
- const defaultActiveId = getItemId( baseId, immutableDefaultValue );
-
- const handleOnChange = ( nextValue: AlignmentMatrixControlValue ) => {
- onChange( nextValue );
- };
-
const compositeStore = useCompositeStore( {
- defaultActiveId,
+ defaultActiveId: getItemId( baseId, defaultValue ),
+ activeId: getItemId( baseId, value ),
+ setActiveId: ( nextActiveId ) => {
+ onChange(
+ getItemValue(
+ baseId,
+ nextActiveId as
+ | AlignmentMatrixControlValue
+ | undefined
+ | null
+ )
+ );
+ },
rtl: isRTL(),
} );
+ const activeId = compositeStore.useState( 'activeId' );
+
const classes = classnames(
'component-alignment-matrix-control',
className
@@ -88,7 +90,6 @@ export function AlignmentMatrixControl( {
store={ compositeStore }
aria-label={ label }
as={ Root }
- id={ baseId }
className={ classes }
role="grid"
size={ width }
@@ -97,7 +98,7 @@ export function AlignmentMatrixControl( {
<CompositeRow as={ Row } role="row" key={ index }>
{ cells.map( ( cell ) => {
const cellId = getItemId( baseId, cell );
- const isActive = cell === currentCell;
+ const isActive = cellId === activeId;
return (
<Cell
@@ -105,7 +106,6 @@ export function AlignmentMatrixControl( {
isActive={ isActive }
key={ cell }
value={ cell }
- onFocus={ () => handleOnChange( cell ) }
/>
);
} ) }
diff --git a/packages/components/src/alignment-matrix-control/stories/index.story.tsx b/packages/components/src/alignment-matrix-control/stories/index.story.tsx
index 24b496d1f2..03bec9d92a 100644
--- a/packages/components/src/alignment-matrix-control/stories/index.story.tsx
+++ b/packages/components/src/alignment-matrix-control/stories/index.story.tsx
@@ -6,7 +6,7 @@ import type { Meta, StoryFn } from '@storybook/react';
/**
* WordPress dependencies
*/
-import { useEffect, useState } from '@wordpress/element';
+import { useState } from '@wordpress/element';
import { Icon } from '@wordpress/icons';
/**
@@ -24,10 +24,11 @@ const meta: Meta< typeof AlignmentMatrixControl > = {
'AlignmentMatrixControl.Icon': AlignmentMatrixControl.Icon,
},
argTypes: {
- onChange: { action: 'onChange', control: { type: null } },
+ onChange: { control: { type: null } },
value: { control: { type: null } },
},
parameters: {
+ actions: { argTypesRegex: '^on.*' },
controls: { expanded: true },
docs: { canvas: { sourceState: 'shown' } },
},
@@ -42,11 +43,6 @@ const Template: StoryFn< typeof AlignmentMatrixControl > = ( {
const [ value, setValue ] =
useState< AlignmentMatrixControlProps[ 'value' ] >();
- // Convenience handler for Canvas view so changes are reflected
- useEffect( () => {
- setValue( defaultValue );
- }, [ defaultValue ] );
-
return (
<AlignmentMatrixControl
{ ...props }
diff --git a/packages/components/src/alignment-matrix-control/types.ts b/packages/components/src/alignment-matrix-control/types.ts
index 892781234e..3b0378401b 100644
--- a/packages/components/src/alignment-matrix-control/types.ts
+++ b/packages/components/src/alignment-matrix-control/types.ts
@@ -27,11 +27,13 @@ export type AlignmentMatrixControlProps = {
/**
* The current alignment value.
*/
- value?: AlignmentMatrixControlValue;
+ value?: AlignmentMatrixControlValue | null;
/**
* A function that receives the updated alignment value.
*/
- onChange?: ( newValue: AlignmentMatrixControlValue ) => void;
+ onChange?: (
+ newValue: AlignmentMatrixControlValue | undefined | null
+ ) => void;
/**
* If provided, sets the width of the control.
*
diff --git a/packages/components/src/alignment-matrix-control/utils.tsx b/packages/components/src/alignment-matrix-control/utils.tsx
index 7f0e2b08e3..8159bf2b89 100644
--- a/packages/components/src/alignment-matrix-control/utils.tsx
+++ b/packages/components/src/alignment-matrix-control/utils.tsx
@@ -37,7 +37,9 @@ export const ALIGNMENTS = GRID.flat();
*
* @return The normalized value
*/
-export function normalizeValue( value: AlignmentMatrixControlValue ) {
+export function normalizeValue(
+ value: AlignmentMatrixControlValue | undefined | null
+) {
return value === 'center' ? 'center center' : value;
}
@@ -48,11 +50,12 @@ export function normalizeValue( value: AlignmentMatrixControlValue ) {
*
* @return The parsed value.
*/
-export function transformValue( value: AlignmentMatrixControlValue ) {
- return normalizeValue( value ).replace(
- '-',
- ' '
- ) as AlignmentMatrixControlValue;
+export function transformValue(
+ value: AlignmentMatrixControlValue | undefined | null
+) {
+ return normalizeValue( value )?.replace( '-', ' ' ) as
+ | AlignmentMatrixControlValue
+ | undefined;
}
/**
@@ -65,9 +68,14 @@ export function transformValue( value: AlignmentMatrixControlValue ) {
*/
export function getItemId(
prefixId: string,
- value: AlignmentMatrixControlValue
+ value: AlignmentMatrixControlValue | undefined | null
) {
- const valueId = transformValue( value ).replace( ' ', '-' );
+ const transformed = transformValue( value );
+ if ( ! transformed ) {
+ return undefined;
+ }
+
+ const valueId = transformed.replace( ' ', '-' );
return `${ prefixId }-${ valueId }`;
}
@@ -79,9 +87,12 @@ export function getItemId(
* @param id An item ID
* @return The item value
*/
-export function getItemValue( prefixId: string, id: string ) {
+export function getItemValue(
+ prefixId: string,
+ id: AlignmentMatrixControlValue | undefined | null
+) {
return transformValue(
- id.replace( prefixId + '-', '' ) as AlignmentMatrixControlValue
+ id?.replace( prefixId + '-', '' ) as AlignmentMatrixControlValue
);
}
@@ -93,9 +104,13 @@ export function getItemValue( prefixId: string, id: string ) {
* @return The index of a matching alignment.
*/
export function getAlignmentIndex(
- alignment: AlignmentMatrixControlValue = 'center'
+ alignment: AlignmentMatrixControlValue | undefined | null = 'center'
) {
const item = transformValue( alignment );
+ if ( ! item ) {
+ return undefined;
+ }
+
const index = ALIGNMENTS.indexOf( item );
return index > -1 ? index : undefined;
Noting that my code changes introduce a slight change of behavior for the onChange prop, which will not fire when the user picks the same value as the currently selected one (meaning that this usecase is expected to fail, since center center is already the default value)
With regards to the tests, I'd love to be able to avoid hacky workarounds — at the same time, I'd be happy to punt that to a follow-up PR and go ahead with the rest of the migration.
Most importantly, though, I noticed that this PR so far introduces the new Composite components from ariakit alongside the old reakit version (under a v2.ts file):
- there are still many components relying on
reakit'sCompositecomponents — those usages should be migrated too - we need to assess the backwards compatibility of the new APIs with the old ones — for example, how can we ensure backwards compat between
useCompositeState(reakit) anduseCompositeStore(ariakit)? @diegohaz I guess this question is more for you — ideally we'd like to remove reakit entirely from the project, in favour of ariakit.
Does it still need to keep backward compatibility considering it's exported as gutenberg/packages/components/src/index.ts Lines 59 to 64 in 21b6a37 If so, the main difference between state and store hooks is that state hooks include all state as direct properties of the state object and re-render the component on every state change. This can be achieved with the store with this code: function useCompositeState() {
const store = useCompositeStore();
const state = store.useState();
return { ...store, ...state };
}Besides that, state hooks accept only the initial state as an argument and some properties have been renamed (check Reakit's function useCompositeState(props) {
const store = useCompositeStore(convertInput(props));
const state = store.useState();
return convertOutput({ ...store, ...state });
} |
packages/components/src/alignment-matrix-control/test/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/alignment-matrix-control/test/index.tsx
Outdated
Show resolved
Hide resolved
@diegohaz unfortunately is it my understanding that any APIs that got merged into WordPress could be assumed to be stable regardless of its prefix (see an example of past conversations), which then led to the creation of the In the case of |
I understand. If I remember correctly, those exports were incorporated into the It would surprise me if this is being used anywhere outside of Gutenberg, particularly since individuals could simply use the stable Reakit API directly, either via npm or CDN. But who knows? |
That is still the case, and an unfortunate consequence of that approach is that the
Unfortunately, there are usages (example WPdirectory search). Since the whole point of this refactor is to remove In my opinion, we have a couple of alternatives:
What do folks think? |
|
Given that |
|
After a quick chat, here's the plan of action:
|
|
Also wanted to flag that, with #54818 merged, |
packages/components/src/alignment-matrix-control/stories/index.story.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
This is great work @andrewhayward 🙌
I've left some feedback, but don't consider any of it blocking. I'm happy to defer to @ciampo and company for a ✅ .
packages/components/src/alignment-matrix-control/test/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
LGTM 🚀
Let's merge this PR and continue with the next steps!
ciampo
added
the
Needs Dev Note
|
Dev Note to be added in #58620. |
mirka
removed
the
Needs Dev Note
What?
As per #53278, we need to move away from
reakitto useariakitinstead. This PR tackles part of that, by starting the migration of theCompositecomponent.Why?
Compositeis currently a simple wrapper aroundreakit, re-exporting its various component parts. It needs to move away from this and exportariakitinstead.How?
This is the first step in migrating
Compositeaway fromreakit:reakittoariakitin@wordpress/components, but without modifying the version exported by the@wordpress/componentspackageariakitversion ofCompositevia the private APIsIt does not tackle the following, which will be follow-up tickets:
reakitcomposite components across Gutenbergreakitimplementation, and use theariakit-based version instead, removing theariakitprivate API exportNote
Even though
Compositeis currently exported from the components package as__unstable, it is being used publicly, so we will have to proceed with caution!Testing Instructions
Despite the API changes, both
AlignmentMatrixControlandCircularOptionPickershould continue to work as now.